Rewrite the maths & uniform API #43

0x00002a · 2024-02-02T03:44:51Z

So theres quite a lot here and I'm aware it might not fly as its a substantial change, though it is mostly non-breaking it does remove some functionality.

Matrix and Matrix3 are gone. My thought process is that there are not many use-cases for the 3x3 stuff since basically all you can use it for is CPU math which the rust ecosystem already has stuff for.

Matrix4's API has been made more explicit about the row order and has been made Copy, also it no longer gives out pointers to the inner as that's just asking for trouble. If unsafe code wants to cast the reference to a pointer that's its business.

Uniform is now an enum rather than a sealed trait. This gives us some nice things:

we express all the possible options up front rather than hidden in many impl's
consumers can make their own types work as uniforms by implementing From<MyType> for Uniform

also I added the missing bindings and fixed the soundness issue where an overflow could occure

glam interop with From impl's for FVec, Matrix4, and Uniform (which lets you bind glam mat4's directly as uniforms), this should help with that CPU math

ian-h-chamberlain

Hey, thanks for this! I think it does simplify things a lot and I think you're right that 3x3 CPU math can be done in plain Rust, or coerced into PICA types for GPU math.

One use case I'm not sure this would support (which maybe the Uniform trait would have) is a custom type that's larger than the default types, e.g.

struct BoolVec([bool; 4]);
struct Mat5x5([[f32; 5]; 5]);
struct CustomIVec([u8; 6]);

I guess maybe the way to handle this kind of thing would be to have a helper function on the type itself, so you'd own a range of indices and call bind multiple times to bind one of these custom types?

Overall I think this change looks reasonable! I had a couple small comments about transmute and stuff like that, but generally I think it makes sense and I'm not opposed to simplifying the API a bit and starting integration with some more common Rust graphics / math libs too. Thanks!

citro3d/src/math/matrix.rs

citro3d/Cargo.toml

ian-h-chamberlain · 2024-02-04T18:42:16Z

citro3d/src/math/matrix.rs

-            // UNWRAP: M ≤ 4, so slicing to a smaller array should always work
-            rows[..M].try_into().unwrap()
-        }
+    pub fn as_raw(&self) -> &citro3d_sys::C3D_Mtx {


nit: doc comment would be nice. Also, do we really need both into_raw and as_raw? I think since C3D_Mtx is Copy we should be able to get away with just one or the other...

as_raw because if we want to pass to the C API we need a reference to the underlying C3D_Mtx. If we're just passing constant pointers which never get held anywhere (which I think might be the case actually) this is fine, but its a potential footgun that I didn't want to think too hard about. I can make it only as_raw(self) -> citro3d_sys::C3D_Mtx if you like?

I guess I don't have a strong opinion either way on this, I would think that as_raw() -> *const citro3d_sys::C3D_Mtx might be more "universal" since you could just deref to make a copy?

citro3d/src/math/matrix.rs

ian-h-chamberlain · 2024-02-04T19:06:16Z

citro3d/src/uniform.rs

+pub struct Index(u8);

-impl From<i8> for Index {
-    fn from(value: i8) -> Self {
+impl From<u8> for Index {


I'm fine with changing the representation to u8 here, but I guess maybe we should implement TryFrom to make sure the index is always in the range 0..=i8::MAX? Not sure what the behavior would be if you passed a negative value or something to the uniform APIs...

you can't because it'd be caught by the checks in Uniform::bind, its u8 cause that makes more sense for an index, <0 isn't valid for them and is only returned by the functions in the C API so it can report error

I guess I'm wondering what would happen if someone did let i = Index::from(200) and then tried to use that index later on in a bind call... Maybe we should remove this From impl entirely and make it so the only way to get an Index is via Program::get_uniform.

I guess I'm wondering what would happen if someone did let i = Index::from(200) and then tried to use that index later on in a bind call... Maybe we should remove this From impl entirely and make it so the only way to get an Index is via Program::get_uniform.

yeah it'd panic on the bind call

actually I think I can make this even safer by pulling the valid register range out of the shader program in get uniform and putting it in index. this would let us assert and prevent binding mismatching uniforms (e.g. trying to bind a 4x4 to a 1x4). should I add that?

That sounds great! The more safe we can make the APIs the better, in my opinion

yeah tried this uh something is going wrong with parsing the shader file but it seems to work weirdly and I can't work it out so I'm going to leave it for now, can always be done later (assuming we keep Index a black box it won't be breaking)

citro3d/src/uniform.rs

0x00002a · 2024-02-04T20:22:35Z

Hey, thanks for this! I think it does simplify things a lot and I think you're right that 3x3 CPU math can be done in plain Rust, or coerced into PICA types for GPU math.

One use case I'm not sure this would support (which maybe the Uniform trait would have) is a custom type that's larger than the default types, e.g.
* `struct BoolVec([bool; 4]);`

* `struct Mat5x5([[f32; 5]; 5]);`

* `struct CustomIVec([u8; 6])`;
I guess maybe the way to handle this kind of thing would be to have a helper function on the type itself, so you'd own a range of indices and call bind multiple times to bind one of these custom types?

So originally I had the float arm of Uniform use an &[FVec4] but I changed it to the fixed version for... reason? after reading the C code I think? There might be a limit of 4 in a row bindings but saying that it doesn't really make sense? so yeah might want to change that back to the &[FVec4] version instead to support this use case, could also switch all the arms to be &[..] (this stops us implementing From for non-references though and its kinda weird).

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

ian-h-chamberlain · 2024-02-04T21:41:33Z

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

Yeah, I guess that's true — I had been hoping it might be possible to implement truly custom uniform types but maybe that's not feasible without implementing everything down to the GPU register writes themselves (like https://github.com/devkitPro/citro3d/blob/master/source/uniforms.c#L54).

I guess let's consider this "out of scope" for now, maybe implementing custom registers is possible but if so it could probably just be another enum variant, so we can deal with that later on.

So originally I had the float arm of Uniform use an &[FVec4] but I changed it to the fixed version for... reason? after reading the C code I think? There might be a limit of 4 in a row bindings but saying that it doesn't really make sense? so yeah might want to change that back to the &[FVec4] version instead to support this use case, could also switch all the arms to be &[..] (this stops us implementing From for non-references though and its kinda weird).

From what I can tell, there isn't necessarily a limit since C3D_FVUnifMtxNx4 seems to allow any num as its input count, but I would be surprised if there are any common use cases for more that 4x4. I was trying to think if we could use Cow<[FVec]> or something, but I don't think that works with array types, only Vec, and we probably don't want to use a Vec for every owned uniform like that.

I think having separate enum variants is fine for now, we could always change it later if it seems like having 5x4 or whatever is useful.

0x00002a · 2024-02-05T11:33:31Z

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

Yeah, I guess that's true — I had been hoping it might be possible to implement truly custom uniform types but maybe that's not feasible without implementing everything down to the GPU register writes themselves (like https://github.com/devkitPro/citro3d/blob/master/source/uniforms.c#L54).

Yeah I think at that point we'd want an entirely new rust-native library for using the GPU, which I've been considering writing for a bit now (given the amount of footguns involved in all the state and globals in citro3d) but that's too much time investment for me right now :p

ian-h-chamberlain · 2024-02-11T16:36:08Z

@0x00002a I think everything looks good to me, are you planning on making any additional changes or should I go ahead and merge this if it passes CI checks?

0x00002a · 2024-02-11T20:26:11Z

@0x00002a I think everything looks good to me, are you planning on making any additional changes or should I go ahead and merge this if it passes CI checks?

Yeah I'm good with this changeset

0x00002a added 6 commits February 2, 2024 03:19

feat: API for IVec

44ed48d

feat: improve uniform API and rewrite Matrix API

97a1d12

feat: add helpful constructors to matrix4

34532ae

feat: glam support

3d2a53d

fix: warning and docs

4eafa19

chore: downgrade glam version to slightly older

057ec98

ian-h-chamberlain reviewed Feb 4, 2024

View reviewed changes

0x00002a added 2 commits February 5, 2024 11:28

fix: uneeded usages of transmute

12d62b3

chore: Uniform -> Self

44e3850

0x00002a added 6 commits February 5, 2024 11:35

fix: not using debug impl for printing Index

58e41fe

fix: Uniform not non-exhaustive

b312575

fix: Uniform missing standard derives

7a5b24c

fix: matrix4 implementing Eq

61f7dbe

chore: improve docs on Uniform

8e117c0

fix: glam feature not in docs

d6ce8cd

ian-h-chamberlain approved these changes Feb 11, 2024

View reviewed changes

fix: tests build

52f0019

ian-h-chamberlain merged commit a57fdf1 into rust3ds:main Feb 18, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the maths & uniform API #43

Rewrite the maths & uniform API #43

0x00002a commented Feb 2, 2024 •

edited

Loading

ian-h-chamberlain left a comment

ian-h-chamberlain Feb 4, 2024

0x00002a Feb 5, 2024 •

edited

Loading

ian-h-chamberlain Feb 11, 2024 •

edited

Loading

ian-h-chamberlain Feb 4, 2024

0x00002a Feb 4, 2024

ian-h-chamberlain Feb 4, 2024

0x00002a Feb 4, 2024

0x00002a Feb 5, 2024

ian-h-chamberlain Feb 11, 2024

0x00002a Feb 11, 2024 •

edited

Loading

0x00002a commented Feb 4, 2024

ian-h-chamberlain commented Feb 4, 2024

0x00002a commented Feb 5, 2024

ian-h-chamberlain commented Feb 11, 2024

0x00002a commented Feb 11, 2024

Rewrite the maths & uniform API #43

Rewrite the maths & uniform API #43

Conversation

0x00002a commented Feb 2, 2024 • edited Loading

ian-h-chamberlain left a comment

Choose a reason for hiding this comment

ian-h-chamberlain Feb 4, 2024

Choose a reason for hiding this comment

0x00002a Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

ian-h-chamberlain Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

ian-h-chamberlain Feb 4, 2024

Choose a reason for hiding this comment

0x00002a Feb 4, 2024

Choose a reason for hiding this comment

ian-h-chamberlain Feb 4, 2024

Choose a reason for hiding this comment

0x00002a Feb 4, 2024

Choose a reason for hiding this comment

0x00002a Feb 5, 2024

Choose a reason for hiding this comment

ian-h-chamberlain Feb 11, 2024

Choose a reason for hiding this comment

0x00002a Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

0x00002a commented Feb 4, 2024

ian-h-chamberlain commented Feb 4, 2024

0x00002a commented Feb 5, 2024

ian-h-chamberlain commented Feb 11, 2024

0x00002a commented Feb 11, 2024

0x00002a commented Feb 2, 2024 •

edited

Loading

0x00002a Feb 5, 2024 •

edited

Loading

ian-h-chamberlain Feb 11, 2024 •

edited

Loading

0x00002a Feb 11, 2024 •

edited

Loading